High-confidence a11y: fix footer contentinfo duplication and main navigation/menu semantics#5639
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes a narrowly scoped accessibility fix in the az_barrio page template by removing a duplicate nested contentinfo landmark while preserving the semantic outer footer landmark.
Changes:
- Removed
role="contentinfo"from the inner footer wrapper. - Kept
<footer class="site-footer">as the single contentinfo region.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Remove nested role=contentinfo inside footer wrapper to keep a single top-level contentinfo landmark and reduce repeated landmark violations. Estimated impact from known scan: addresses landmark-contentinfo-is-top-level (100) and landmark-no-duplicate-contentinfo (99), roughly 199 findings (~40% of the prior 500). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use semantic list markup for submenu containers in main navigation template and add explicit primary navigation landmark label in page layout. This is a scoped, high-confidence accessibility fix pass to reduce list and landmark uniqueness violations without altering business logic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
accesswatch
force-pushed
the
accessibility-fixes-high-confidence
branch
from
ed1d090 to
9b1d4ff
Compare
joeparsons
left a comment
joeparsons
left a comment
There was a problem hiding this comment.
Overall these changes look good to me. We'll just want to revert the changes to the composer audit job in the CI workflow before merging.
joeparsons
added
bug
github-project-automation
Bot
moved this to Needs review
in 3.4.2 bug-fix patch release
joeparsons
added
the
patch release
github-project-automation
Bot
moved this from Todo
to Needs review
in 3.3.7 bug-fix patch release
| {% if menu_level == 0 %} | ||
| </li> | ||
| {% else %} | ||
| </li> |
There was a problem hiding this comment.
If both if/else cases result in <li>, if-else isn't needed here.
There was a problem hiding this comment.
Agreed. I'm wondering if the conditional </ul> following this is also now not needed.
There was a problem hiding this comment.
I'm a little confused; the PR description says:
Removed nested
role="contentinfo"inside footer wrapper.
But it still appears here with no change.
Looking at main's version of page.html.twig doesn't seem to show a duplicate role="contentinfo". Does that mean this PR's description and title is outdated?
From what I can tell, the changes here are unrelated to the footer landmark. If that's the case, I'd suggest updating the title and description to more accurately reflect the changes within (i.e., remove references to footer landmark changes since there are none here).
| {% if menu_level == 0 %} | ||
| </li> | ||
| {% else %} | ||
| </li> |
There was a problem hiding this comment.
Agreed. I'm wondering if the conditional </ul> following this is also now not needed.
Addresses review feedback (@djcelaya, @joshuasosa): - Remove the nested role="contentinfo" on the inner footer div. It was applied earlier in this PR, then accidentally re-added in a later commit; <footer class="site-footer"> is the single contentinfo landmark again. - Collapse the redundant menu_level if/else around the closing </li> and </ul> in menu--main.html.twig — both branches were identical after the dropdown <div> -> <ul> change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
accesswatch
changed the title
|
Addressed the feedback in 441d0aa:
Re-requesting review. 🙏 |
|
@djcelaya this is ready for another look. All of your suggestions are in (441d0aa): the footer |
github-project-automation
Bot
moved this from Needs review
to Ready to merge
in 3.4.4 bug-fix patch release
github-project-automation
Bot
moved this from Ready to merge
to Done
in 3.4.4 bug-fix patch release
Narrowly-scoped, high-confidence accessibility fixes in the
az_barriotheme.Changes
role="contentinfo"on the innerfooter
<div>inlayout/page.html.twig.<footer class="site-footer">isalready an implicit
contentinfolandmark, so the explicit role created aduplicate landmark. (This fix was applied earlier in the PR and then
accidentally re-added in a later commit; it is now re-applied.)
aria-label="Primary navigation"to thetop
<nav>so the navigation landmark is named.<div class="dropdown-menu">to a proper<ul>/<li>list, and collapsethe now-redundant
menu_levelif/elsearound the closing</li>and</ul>(both branches were identical after the change).Review
Addresses @djcelaya and @joshuasosa: the title/description now match the diff,
the accidentally-reverted footer fix is back, and the redundant menu
conditionals are removed.